feat(web): memory management in web ui#127
feat(web): memory management in web ui#127skulldogged wants to merge 5 commits intospacedriveapp:mainfrom
Conversation
WalkthroughAdds memory create/update/delete endpoints and client methods, frontend dialogs and mutations for editing/removing memories, backend handlers for create/update/delete with validation, embedding management and FTS updates, and a multi-method /agents/memories route supporting GET/POST/PUT/DELETE. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
interface/src/routes/AgentMemories.tsx (2)
272-274:virtualizerin the dependency array may be unnecessary.
useVirtualizerreturns a stable instance (ref-backed), so including it won't cause an infinite loop, but the effect is really driven bymemories.lengthandexpandedId. Addingvirtualizerjust satisfies the exhaustive-deps lint — which is fine, but worth knowing it's inert.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/routes/AgentMemories.tsx` around lines 272 - 274, The effect calls virtualizer.measure() but the dependency array only needs to react to changes in memories.length and expandedId; virtualizer is a stable ref from useVirtualizer and can be removed from the dependencies to avoid a noisy lint-driven inclusion. Update the useEffect in AgentMemories.tsx to depend only on [memories.length, expandedId] (or if you must keep the lint happy, remove virtualizer and add a single-line eslint-disable comment above the useEffect for react-hooks/exhaustive-deps), leaving the call to virtualizer.measure() unchanged.
177-260: Consider passing form data as themutateargument instead of closing over component state.
createMutationandupdateMutationreadformDataandeditingMemoryfrom the component closure rather than receiving them asmutate(…)arguments. This works today becausesaveMemoryvalidates and callsmutate()synchronously in the same render, but it's fragile if the flow ever becomes async (e.g., adding a confirmation step).♻️ Example: passing form data through mutate
const createMutation = useMutation({ - mutationFn: () => { - return api.createMemory({ - agent_id: agentId, - content: formData.content, - memory_type: formData.memory_type, - importance: formData.importance, - }); - }, + mutationFn: (data: MemoryFormData) => { + return api.createMemory({ + agent_id: agentId, + content: data.content, + memory_type: data.memory_type, + importance: data.importance, + }); + }, ... }); const updateMutation = useMutation({ - mutationFn: () => { - if (!editingMemory) throw new Error("No memory selected"); - return api.updateMemory({ - agent_id: agentId, - memory_id: editingMemory.id, - content: formData.content, - memory_type: formData.memory_type, - importance: formData.importance, - }); - }, + mutationFn: ({ memoryId, data }: { memoryId: string; data: MemoryFormData }) => { + return api.updateMemory({ + agent_id: agentId, + memory_id: memoryId, + content: data.content, + memory_type: data.memory_type, + importance: data.importance, + }); + }, ... });Then in
saveMemory:if (editingMemory) { - updateMutation.mutate(); + updateMutation.mutate({ memoryId: editingMemory.id, data: formData }); } else { - createMutation.mutate(); + createMutation.mutate(formData); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/routes/AgentMemories.tsx` around lines 177 - 260, createMutation and updateMutation close over component state (formData, editingMemory) which is fragile; modify both mutations so their mutationFn accepts an input argument (e.g., variables) and uses that to call api.createMemory / api.updateMemory instead of reading formData/editingMemory from closure, and then change saveMemory to build a plain payload (trimmed content, memory_type, importance, and for updates include editingMemory.id) and call createMutation.mutate(payload) or updateMutation.mutate(payload); keep the same success/error handlers (refreshMemories, setEditorOpen, setFormError, etc.) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/api/memories.rs`:
- Around line 195-226: The handler currently calls store.save(&memory) before
creating/storing the embedding, which leaves a memory persisted without an
embedding if embed_one or embedding_table().store fails; either move the
embedding step before persisting (call
embedding_model_arc().embed_one(&memory.content) first, then store.save(&memory)
and embedding_table().store) or, if persisting first is required, add a rollback
on failure (on any error from embed_one or embedding_table().store call
store.delete(&memory.id) or equivalent to remove the partially saved memory),
and keep the existing ensure_fts_index() call after successful storage; update
the code paths around store.save, embedding_model_arc().embed_one,
embedding_table().store, and error handling to implement one of these fixes.
---
Nitpick comments:
In `@interface/src/routes/AgentMemories.tsx`:
- Around line 272-274: The effect calls virtualizer.measure() but the dependency
array only needs to react to changes in memories.length and expandedId;
virtualizer is a stable ref from useVirtualizer and can be removed from the
dependencies to avoid a noisy lint-driven inclusion. Update the useEffect in
AgentMemories.tsx to depend only on [memories.length, expandedId] (or if you
must keep the lint happy, remove virtualizer and add a single-line
eslint-disable comment above the useEffect for react-hooks/exhaustive-deps),
leaving the call to virtualizer.measure() unchanged.
- Around line 177-260: createMutation and updateMutation close over component
state (formData, editingMemory) which is fragile; modify both mutations so their
mutationFn accepts an input argument (e.g., variables) and uses that to call
api.createMemory / api.updateMemory instead of reading formData/editingMemory
from closure, and then change saveMemory to build a plain payload (trimmed
content, memory_type, importance, and for updates include editingMemory.id) and
call createMutation.mutate(payload) or updateMutation.mutate(payload); keep the
same success/error handlers (refreshMemories, setEditorOpen, setFormError, etc.)
unchanged.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
interface/src/api/client.tsinterface/src/routes/AgentMemories.tsxsrc/api/memories.rssrc/api/server.rs
Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/api/memories.rs`:
- Around line 197-200: The handler is awaiting synchronous memory persistence
calls (store.save / store.update / store.forget), which blocks the request path;
change these to fire-and-forget by spawning a background task with tokio::spawn
that owns/clones any needed state (e.g., the Store, memory object,
request.agent_id, embedding data) and performs store.save(...).await inside the
spawned task while logging failures with tracing::warn/error there; ensure the
handler returns immediately after spawning the task. Apply the same pattern for
the other occurrences (store.update, store.forget and any embedding writes
referenced in the file).
- Around line 256-305: The handler updates the Memory record before generating
and storing the new embedding, which can leave search inconsistent if embedding
creation/storage fails; change the flow in the content update branch so you
first generate the new embedding via
memory_search.embedding_model_arc().embed_one(&memory.content).await, then
persist the embedding with memory_search.embedding_table().store(&memory.id,
&memory.content, &embedding).await, and only after successful embedding
persistence call store.update(&memory).await (ensuring memory.updated_at is set
just prior to the final update). Do not call
memory_search.embedding_table().delete(...) before the new embedding is safely
stored; instead delete the old embedding only after the new one is persisted,
and if any embedding persistence step fails revert any in-memory changes (i.e.,
do not call store.update or roll back by restoring the previous Memory in the
store) so the DB and embedding index remain consistent.
Summary
Memoriesview./api/agents/memories(POST/PUT/DELETE), including embedding sync for create/update/delete so search stays consistent.Details
src/api/memories.rsfor create, update, and soft-delete (forget) flows.src/api/server.rsunder/agents/memories.interface/src/api/client.ts.interface/src/routes/AgentMemories.tsxwith: